Skip to content

Remove 1 TODO from ConditionalMutation.java#6463

Merged
ctubbsii merged 9 commits into
apache:mainfrom
Amemeda:mutation-todo
Jul 9, 2026
Merged

Remove 1 TODO from ConditionalMutation.java#6463
ctubbsii merged 9 commits into
apache:mainfrom
Amemeda:mutation-todo

Conversation

@Amemeda

@Amemeda Amemeda commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Removed TODO add ByteSequence methods to mutations from line 63 of ConditionalMutation.java with no other modifications

After some discussion, the TODO was unnecessary, initial changes in this pr were reverted.


REVERTED CHANGES FOR REFERENCE:

Updated Mutation.java

  • Added new constructor with ByteSequence row parameter
  • Added new get method to get row as a ByteSequence

Updated ConditionalMutation.java

  • removed TODO
  • called new Mutation constructor with ByteSequence parameter

Updated MutationTest.java

  • Added new test testByteSequenceConstructor

@Amemeda

Amemeda commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Removes 1 TODO from #2699

@Amemeda Amemeda marked this pull request as ready for review July 7, 2026 19:21
Comment thread core/src/main/java/org/apache/accumulo/core/data/Mutation.java Outdated
Comment thread core/src/main/java/org/apache/accumulo/core/data/Mutation.java Outdated
@Amemeda Amemeda requested a review from DomGarguilo July 9, 2026 18:44
@Amemeda Amemeda changed the title Adding new Mutation constructor with ByteSequence parameter to remove 1 TODO Remove 1 TODO from ConditionalMutation.java Jul 9, 2026
@ctubbsii

ctubbsii commented Jul 9, 2026

Copy link
Copy Markdown
Member

This probably doesn't need to be done and the TODO can just be removed. Updating the public API creates churn that can be disruptive and more to maintain. I can see that it might have been worth doing this if somehow Mutation was altered to keep a reference to the ByteSequence and used that when it was serialized, instead of merely moving the toArray into a constructor, in order to possibly avoid an array copy, but it's not really necessary, and easier to just remove the TODO.

@ctubbsii ctubbsii merged commit 1728a61 into apache:main Jul 9, 2026
4 checks passed
@ctubbsii ctubbsii added this to the 4.0.0 milestone Jul 9, 2026
@Amemeda Amemeda deleted the mutation-todo branch July 9, 2026 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants